-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for MAX14906 and MAX14916 industrial IO with diagnostics #74891
base: main
Are you sure you want to change the base?
Conversation
6059494
to
78c5fe0
Compare
type: boolean | ||
spi-addr: | ||
type: int | ||
default: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to explain in the description why the default value was selected
https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#dt-bindings-default-rules
Same for the following properties below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks , explanation is added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to be done for the properties below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added default values are from documentation - default value per register
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, why is this property needed? It's redundant to the reg
property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using reg
for that purpose will be kind of confusing.
spi-addr
is something specific to MAX14906 and MAX14916.
type: boolean | ||
spi-addr: | ||
type: int | ||
default: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to explain in the description why the default value was selected
https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#dt-bindings-default-rules
Same for the properties below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explanation added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to be done for the properties below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added default values are from documentation - default value per register
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, why is this property needed? It's redundant to the reg
property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using reg
for that purpose will be kind of confusing.
spi-addr
is something specific to MAX14906 and MAX14916.
ef4c38b
to
4d07d53
Compare
4d07d53
to
7fa2e0e
Compare
drivers/gpio/gpio_max14906.h
Outdated
union max14906_doi_level { | ||
uint8_t reg_raw; | ||
struct { | ||
uint8_t VDDOK_FAULT1:1; /* BIT0 */ | ||
uint8_t VDDOK_FAULT2:1; | ||
uint8_t VDDOK_FAULT3:1; | ||
uint8_t VDDOK_FAULT4:1; | ||
uint8_t SAFE_DAMAGE_F1:1; | ||
uint8_t SAFE_DAMAGE_F2:1; | ||
uint8_t SAFE_DAMAGE_F3:1; | ||
uint8_t SAFE_DAMAGE_F4:1; /* BIT7 */ | ||
} reg_bits; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is obvious that bitfield and union simplify driver implementation.
I wonder does the zephyr encourage using bitfield @MaureenHelm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is obvious that bitfield and union simplify driver implementation.
I wonder does the zephyr encourage using bitfield @MaureenHelm
I don't think it should block this PR.
* @param val - Value which is to be written to requested register. | ||
* @return 0 in case of success, negative error code otherwise. | ||
*/ | ||
static int max149x6_reg_transceive(const struct device *dev, uint8_t addr, uint8_t val, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this implemented in the header file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it is shared from MAX14906 and MAX14916
* @param encode - action to be performed - true(encode), false(decode) | ||
* @return the resulted CRC5 | ||
*/ | ||
static uint8_t max149x6_crc(uint8_t *data, bool encode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this implemented in the header file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it is shared from MAX14906 and MAX14916
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about to to rename it as .c instead .h to it be more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if will be more readable. I prefer to keep it with .h to avoid confusion if max149x6 is separate driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok, thanks.
type: boolean | ||
spi-addr: | ||
type: int | ||
default: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, why is this property needed? It's redundant to the reg
property
- 0 mean enable | ||
- 1 mean disable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? I think it may be inverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is inverted. Unfortunately multiplicative by copy/paste. Fixed on all locations.
- 0 mean enable | ||
- 1 mean disable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? I think it may be inverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is inverted. Unfortunately multiplicative by copy/paste. Fixed on all locations.
- 0 mean enable | ||
- 1 mean disable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? I think it may be inverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is inverted. Unfortunately multiplicative by copy/paste. Fixed on all locations.
drivers/gpio/gpio_max14916.c
Outdated
|
||
if (!gpio_pin_get_dt(&config->fault_gpio)) { | ||
LOG_ERR("FLT flag is rised"); | ||
ret = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use an errno
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
drivers/gpio/gpio_max14916.c
Outdated
if (data->glob.interrupt.reg_bits.COM_ERR) { | ||
LOG_ERR("MAX14916 Communication Error"); | ||
} | ||
ret = -2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use an errno
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
type: boolean | ||
spi-addr: | ||
type: int | ||
default: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, why is this property needed? It's redundant to the reg
property
7fa2e0e
to
dcbd148
Compare
@bogdanovs please rebase |
dcbd148
to
11d7f63
Compare
@MaureenHelm branch is rebased on latest main |
struct max149x6_config { | ||
struct spi_dt_spec spi; | ||
struct gpio_dt_spec fault_gpio; | ||
struct gpio_dt_spec ready_gpio; | ||
struct gpio_dt_spec sync_gpio; | ||
struct gpio_dt_spec en_gpio; | ||
bool crc_en; | ||
union max14906_config1 config1; | ||
union max14906_config2 config2; | ||
union max14906_config_curr_lim curr_lim; | ||
union max14906_config_di config_do; | ||
union max14906_config_do config_di; | ||
enum max149x6_spi_addr spi_addr; | ||
uint8_t pkt_size; | ||
}; | ||
|
||
#define max14906_config max149x6_config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason not using typedef?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly because of @MaureenHelm comment here #71141 (comment)
I personally prefer them but since they are not very welcomed in Zephyr that is fine by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to point it.
drivers/gpio/gpio_max14906.c
Outdated
#define max14906_reg_read(dev, addr) max14906_reg_trans_spi_diag(dev, addr, 0, MAX149x6_READ) | ||
#define max14906_reg_write(dev, addr, val) max14906_reg_trans_spi_diag(dev, addr, val, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fine for me but might be not match with naming convention for macro
https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-3-macro-name-collisions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just read the article , but I am not sure what you are referring to. @ozersa can you elaborate please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems macros defined with upper case (MIN, MAX, ARRAY_SIZE...) that make it more readable, when looking the code it clearly mention either it is macro or not when reading source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok yes, that make sens. I thought you are talking about how macros names are worded.
Will replace them with upper case.
e41f5fe
to
83e3d37
Compare
drivers/gpio/gpio_max149x6.h
Outdated
#define MAX149x6_READ 0 | ||
#define MAX149x6_WRITE 1 | ||
|
||
#define MAX149X6_GET_BIT(val, i) (0x1 & ( (val) >> (i))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ozersa here is GET_BIT define
ae203ac
to
ac70f34
Compare
en-gpios = <&test_gpio 0 0>; | ||
}; | ||
|
||
>>>>>>> 60f39fa91c5 (drivers: gpio: Add MAX14906 industrial input/output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. Thanks !
Fixed.
I am wondering : shouldn't checkpatch catch it ?
MAX14906 in 4 channel I/O with advanced diagnostic. In SPI communication diagnostic status transmitted on every READ/WRITE which includes generic status of chip. Configuration both on global level and on per channel bases. Diagnostics includes : * Thermal overload * current limit * open wire detection * short to VDD * Above VDD * Safe DEmagnitization fault * VDD warning * VDD low * SPI/CRC Error * WDog Error * Loss GND Add app.overlay for MAX14906 driver. Tested with adopted basic/button and basic/blinky sample. Signed-off-by: Stoyan Bogdanov <[email protected]>
MAX14906 industrial 4 channel Input/Ouput GPIO expander with diagnostics. Per channel diagnostics for open wire, over current. Global diagnostic for power supply, communication and various fault conditions. Signed-off-by: Stoyan Bogdanov <[email protected]>
Industrial 8 channel output with advanced diagnostics. Allowing giagnostic configuration both on per channel or global bases In SPI communication diagnostic status transmitted on every READ/WRITE which includes generic status of chip. Diagnostics includes : * Oveload * Open Wire * Over current * Short to VDD * Thermal Shutdown * VDD Warn * Watch Dog Error * Communication Error * VDD under voltage Add app.overlay for MAX14916 driver. Tested with adopted basic/blinky example. Signed-off-by: Stoyan Bogdanov <[email protected]>
Industrial 8 channel input GPIO expander with diagnostics Per channel diagnostics from dtb Signed-off-by: Stoyan Bogdanov <[email protected]>
ac70f34
to
fc16f7c
Compare
Add MAX14906 and MAX14916 drivers with diagnostics
MAX14906 in 4 channel I/O with advanced diagnostic.
In SPI communication diagnostic status transmitted on every
READ/WRITE which includes generic status of chip.
Configuration both on global level and on per channel bases.
Tested with modified basic/blinky and basic/button examples.
MAX14916 Industrial 8 channel output with advanced diagnostics.
Allowing giagnostic configuration both on per channel or global bases
In SPI communication diagnostic status transmitted on every
READ/WRITE which includes generic status of chip.
Tested with modified basic/blinky example.
Both chips have advanced diagnostics which include: